refactor (backup) : conditionally copy registry auth secret from operator namespace to workspace namespace for backup/restore#1618
Conversation
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors registry auth secret handling in the backup path. ChangesRegistry Auth Secret Backup-Path Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rohanKanojia The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
56c6857 to
f6f485c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1618 +/- ##
==========================================
+ Coverage 35.48% 37.15% +1.67%
==========================================
Files 168 168
Lines 14484 14764 +280
==========================================
+ Hits 5139 5486 +347
+ Misses 9006 8923 -83
- Partials 339 355 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
3 times, most recently
from
802c09c to
87f720c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
controllers/backupcronjob/backupcronjob_controller_test.go (1)
312-314: ⚡ Quick winAdd one unset/false-path test for
CopyOperatorAuthSecret.All touched
executeBackupSyncfixtures now forceCopyOperatorAuthSecret=true, so this suite no longer validates the new default/disabled branch. Please add at least one case for unset/false (especially missing workspace secret) to guard the new behavior.Also applies to: 345-346, 403-404, 440-441, 482-483, 514-516, 551-553, 588-589
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/backupcronjob/backupcronjob_controller_test.go` around lines 312 - 314, Tests in backupcronjob_controller_test.go currently set CopyOperatorAuthSecret = pointer.Bool(true) in all executeBackupSync fixtures, so the default/disabled branch (unset/false) and the missing workspace secret path are not covered; add at least one test case for executeBackupSync where CopyOperatorAuthSecret is either nil (unset) or pointer.Bool(false) and the workspace secret is absent, then assert the controller follows the disabled behavior (e.g., does not attempt to copy operator auth and returns the expected status/condition). Update the relevant table/fixtures used by TestExecuteBackupSync (or similarly named test harness) to include this false/unset case and verify expected outcomes for missing workspace secret to guard the new branch.deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml (1)
222-240: ⚡ Quick winSet an explicit CRD default for
copyOperatorAuthSecret.Line 239 says the field defaults to false, but the schema currently has no
default: false. Adding it makes the API contract enforceable and avoids nil/merge ambiguity when the field is omitted.Suggested schema tweak
copyOperatorAuthSecret: + default: false description: |- CopyOperatorAuthSecret controls whether the operator should copy the authentication secret from the operator namespace to the workspace namespace when it's not found in the workspace namespace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml` around lines 222 - 240, The CRD schema for the boolean field copyOperatorAuthSecret currently documents "Defaults to false" but lacks an explicit default; update the OpenAPI schema for the field copyOperatorAuthSecret (the property under the spec schema) to include default: false so the CRD enforces the default value when the field is omitted, ensuring API server/merging behavior matches the documented contract.pkg/secrets/backup.go (2)
101-176: 💤 Low valueLogic and race handling look correct.
EnsureRegistryAuthSecretcorrectly: (1) never overwrites a user-provided secret, (2) returns a clear, actionable error whencopyOperatorAuthSecretis false, (3) handles the Get→Create race viaIsAlreadyExistsre-fetch, and (4) sets the watch label and controller reference on the created secret.Two minor observations (no action required, just flagging for awareness):
- The relevant code snippet from
pkg/provision/sync/sync.goshowsSyncObjectWithClusterprovides diff/update behavior. Bypassing it here is the right call since the explicit goal is to never overwrite — just make sure no future caller expects this function to reconcile drift in the workspace secret (e.g., if the operator's source secret rotates, the workspace copy will not be updated).- The error message at lines 125–134 references
DevWorkspaceBackupAuthSecretName(the predefined name) for both the "not found" name and the "please create" name. That's consistent with the rest of the function but means the user-facing message will not mention the configuredsecretNamefromRegistry.AuthSecret, which can be confusing if the admin configured a custom name. Consider including both names in the error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 101 - 176, Update the error returned when copyOperatorAuthSecret is false in EnsureRegistryAuthSecret to include both the workspace secret name (constants.DevWorkspaceBackupAuthSecretName) and the configured/operator secret name (use sourceSecret.Name) so users see which configured auth secret name the operator expects versus the name missing in the workspace; change the fmt.Errorf in the !copyOperatorAuthSecret branch inside EnsureRegistryAuthSecret to format and include sourceSecret.Name alongside constants.DevWorkspaceBackupAuthSecretName.
30-30: ⚡ Quick winReplace deprecated
k8s.io/utils/pointerwithk8s.io/utils/ptr.
golangci-lint(SA1019) reports thatk8s.io/utils/pointeris deprecated. Useptr.Derefinstead ofpointer.BoolDeref.♻️ Proposed fix
- "k8s.io/utils/pointer" + "k8s.io/utils/ptr"- copyOperatorAuthSecret := pointer.BoolDeref( - dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, - false, - ) + copyOperatorAuthSecret := ptr.Deref( + dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, + false, + )k8s.io/utils/ptr Deref function signature🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` at line 30, Replace the deprecated import and calls to pointer.*: change the import "k8s.io/utils/pointer" to "k8s.io/utils/ptr" and update call sites using pointer.BoolDeref (and any other pointer.XDeref) to use ptr.Deref instead, e.g. replace pointer.BoolDeref(someBoolPtr, false) with ptr.Deref(someBoolPtr, false) in the functions/methods in pkg/secrets/backup.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`:
- Around line 226-244: The CRD documents that copyOperatorAuthSecret defaults to
false but the schema lacks an enforced default; add a JSON schema default by
setting default: false for the copyOperatorAuthSecret property in the CRD (the
YAML under copyOperatorAuthSecret) so the API server will apply the default. If
the CRD is generated from Go types, add the kubebuilder default marker (e.g., //
+kubebuilder:default=false) to the Go field named CopyOperatorAuthSecret (or
copyOperatorAuthSecret) in the API type, regenerate the CRD manifests, and
verify the generated CRD includes default: false under the
copyOperatorAuthSecret schema.
In `@deploy/deployment/openshift/combined.yaml`:
- Around line 226-244: The CRD OpenAPI schema for the copyOperatorAuthSecret
boolean lacks an explicit default; add "default: false" to the property
definition in the combined.yaml CRD schema so the OpenAPIv3 spec matches the
documented and Go behavior (pointer.BoolDeref(..., false)); also update any CRD
description if needed to note this change is a breaking behavioral change for
users who relied on true.
In
`@deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`:
- Around line 223-224: The sentence about the
"controller.devfile.io/watch-secret=true" label is ambiguous: update the text
that references authSecret to state this label requirement only applies to
operator-managed (copied) secrets when copyOperatorAuthSecret is enabled (i.e.,
the operator-created/copy-source path), and clarify that user-provided/manual
authSecret values do not need that label; reference the fields authSecret and
copyOperatorAuthSecret and explicitly mention "operator-managed/copy-source
secrets" and "user-provided/manual secrets" so readers know which secrets must
have controller.devfile.io/watch-secret=true.
In
`@deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml`:
- Around line 224-242: The CRD schema for the boolean property
copyOperatorAuthSecret declares "Defaults to false" in the description but lacks
an OpenAPI default; update the CRD schema for the copyOperatorAuthSecret
property by adding default: false alongside its type: boolean (i.e., under the
copyOperatorAuthSecret property in the CRD's schema) so the generated API docs
and schema reflect the described default behavior.
In `@pkg/secrets/backup.go`:
- Around line 57-61: The change introduced a breaking default (currently using
pointer.BoolDeref(..., false)) for
dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret which
stops auto-copying the operator namespace secret; restore backward-compatible
behavior by changing the default to true in the copyOperatorAuthSecret
initialization (i.e., use pointer.BoolDeref(..., true)) so existing clusters
continue to auto-copy, and if you intentionally want false instead, update
docs/changelog and tests that rely on auto-copy (e.g.,
controllers/backupcronjob/backupcronjob_controller_test.go) to avoid upgrade
surprises.
---
Nitpick comments:
In `@controllers/backupcronjob/backupcronjob_controller_test.go`:
- Around line 312-314: Tests in backupcronjob_controller_test.go currently set
CopyOperatorAuthSecret = pointer.Bool(true) in all executeBackupSync fixtures,
so the default/disabled branch (unset/false) and the missing workspace secret
path are not covered; add at least one test case for executeBackupSync where
CopyOperatorAuthSecret is either nil (unset) or pointer.Bool(false) and the
workspace secret is absent, then assert the controller follows the disabled
behavior (e.g., does not attempt to copy operator auth and returns the expected
status/condition). Update the relevant table/fixtures used by
TestExecuteBackupSync (or similarly named test harness) to include this
false/unset case and verify expected outcomes for missing workspace secret to
guard the new branch.
In
`@deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml`:
- Around line 222-240: The CRD schema for the boolean field
copyOperatorAuthSecret currently documents "Defaults to false" but lacks an
explicit default; update the OpenAPI schema for the field copyOperatorAuthSecret
(the property under the spec schema) to include default: false so the CRD
enforces the default value when the field is omitted, ensuring API
server/merging behavior matches the documented contract.
In `@pkg/secrets/backup.go`:
- Around line 101-176: Update the error returned when copyOperatorAuthSecret is
false in EnsureRegistryAuthSecret to include both the workspace secret name
(constants.DevWorkspaceBackupAuthSecretName) and the configured/operator secret
name (use sourceSecret.Name) so users see which configured auth secret name the
operator expects versus the name missing in the workspace; change the fmt.Errorf
in the !copyOperatorAuthSecret branch inside EnsureRegistryAuthSecret to format
and include sourceSecret.Name alongside
constants.DevWorkspaceBackupAuthSecretName.
- Line 30: Replace the deprecated import and calls to pointer.*: change the
import "k8s.io/utils/pointer" to "k8s.io/utils/ptr" and update call sites using
pointer.BoolDeref (and any other pointer.XDeref) to use ptr.Deref instead, e.g.
replace pointer.BoolDeref(someBoolPtr, false) with ptr.Deref(someBoolPtr, false)
in the functions/methods in pkg/secrets/backup.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58f3b582-37ca-446d-9ace-6bf37dec79a5
📒 Files selected for processing (12)
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.goapis/controller/v1alpha1/zz_generated.deepcopy.gocontrollers/backupcronjob/backupcronjob_controller_test.godeploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yamldeploy/deployment/kubernetes/combined.yamldeploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/deployment/openshift/combined.yamldeploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yamlpkg/config/sync.gopkg/secrets/backup.gopkg/secrets/backup_test.go
| The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be | ||
| recognized by the operator. |
There was a problem hiding this comment.
Clarify label requirement scope for user-provided secrets
Line 223 currently reads as if all authSecret values must have controller.devfile.io/watch-secret=true. That conflicts with the manual-secret path when copyOperatorAuthSecret is disabled. Please scope this sentence to operator-managed/copy-source secrets only.
✏️ Proposed wording tweak
- The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be
- recognized by the operator.
+ For operator-managed secret copying, the source secret in the operator namespace must contain
+ "controller.devfile.io/watch-secret=true" so the operator can recognize and manage it.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be | |
| recognized by the operator. | |
| For operator-managed secret copying, the source secret in the operator namespace must contain | |
| "controller.devfile.io/watch-secret=true" so the operator can recognize and manage it. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`
around lines 223 - 224, The sentence about the
"controller.devfile.io/watch-secret=true" label is ambiguous: update the text
that references authSecret to state this label requirement only applies to
operator-managed (copied) secrets when copyOperatorAuthSecret is enabled (i.e.,
the operator-created/copy-source path), and clarify that user-provided/manual
authSecret values do not need that label; reference the fields authSecret and
copyOperatorAuthSecret and explicitly mention "operator-managed/copy-source
secrets" and "user-provided/manual secrets" so readers know which secrets must
have controller.devfile.io/watch-secret=true.
87f720c to
0f8ddda
Compare
| // If true: The operator will copy the secret from the operator namespace | ||
| // if it's not found in the workspace namespace. This provides automatic configuration | ||
| // but exposes operator-level credentials to workspace users. |
There was a problem hiding this comment.
I think it's a bit verbose,
Can we update thee If true... section to:
If true, this enables the fallback mechanism where the operator will copy the secret from the operator namespace.
| // | ||
| // If false (default): The operator will not copy the secret. Users must manually create a secret | ||
| // with the configured name in their workspace namespace. This is more secure as it allows | ||
| // users to provide scoped credentials with minimal privileges. |
There was a problem hiding this comment.
I think it's a bit verbose,
Can we update the If false... section to:
If false, the operator will not copy the secret to workspace namespaces.
| // users to provide scoped credentials with minimal privileges. | ||
| // | ||
| // Note: Regardless of this setting, if a secret already exists in the workspace namespace, | ||
| // it will never be overwritten. User-provided secrets are always respected. |
There was a problem hiding this comment.
| // it will never be overwritten. User-provided secrets are always respected. | |
| // it will never be overwritten. |
| // Extract flag value (default: false, users must provide their own secret) | ||
| copyOperatorAuthSecret := pointer.BoolDeref( | ||
| dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, | ||
| false, |
There was a problem hiding this comment.
Could we instead define the default field here?
There was a problem hiding this comment.
Initially, I also thought about it, but then hesitated as RegistryConfig wasn't defined in defaults.go as Registry.Path is a required value with no sensible default.
If we add configuration like this for the default value:
BackupCronJob: &v1alpha1.BackupCronJobConfig{
Enable: pointer.Bool(false),
Schedule: "0 0 1 * *",
BackoffLimit: pointer.Int32(1),
Registry: &v1alpha1.RegistryConfig{
Path: "", // Required but no default
AuthSecret: "",
CopyOperatorAuthSecret: pointer.Bool(false),
},
},But this would generate an incomplete/invalid configuration.
Do you think I should proceed with this approach? Or create a named constant for false value in the current line?
| return nil, fmt.Errorf( | ||
| "registry auth secret %q not found in workspace namespace %q. "+ | ||
| "copyOperatorAuthSecret is set to false, so the operator will not copy the secret. "+ | ||
| "Please manually create a secret of type kubernetes.io/dockerconfigjson with the name %q "+ |
There was a problem hiding this comment.
| "Please manually create a secret of type kubernetes.io/dockerconfigjson with the name %q "+ | |
| "Please create a secret of type kubernetes.io/dockerconfigjson with the name %q "+ |
| if !copyOperatorAuthSecret { | ||
| return nil, fmt.Errorf( | ||
| "registry auth secret %q not found in workspace namespace %q. "+ | ||
| "copyOperatorAuthSecret is set to false, so the operator will not copy the secret. "+ |
There was a problem hiding this comment.
| "copyOperatorAuthSecret is set to false, so the operator will not copy the secret. "+ | |
| "copyOperatorAuthSecret is set to false, the secret will not be copied. "+ |
|
@rohanKanojia on second thought, is there ever a use case where the admin sets an Because, the only purpose of the admin supplying the Also in general the secret name provided in |
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
0f8ddda to
43f4862
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/secrets/backup.go (1)
63-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
AuthSecretname is silently ignored on the workspace-namespace fallback path — confirm UX.The function uses two different names depending on the path:
- Backup path (line 66,
lookupName = secretName): the workspace namespace is searched under the operator-configuredAuthSecretname.- Restore path (line 68) and the
EnsureRegistryAuthSecretwrite path (line 108): the predefinedconstants.DevWorkspaceBackupAuthSecretNameis used.So when an admin sets
authSecret: my-secretandcopyOperatorAuthSecret: false, and the workspace user has not pre-created any secret, the workspace lookup at line 71–79 will succeed only if they createdmy-secret, butEnsureRegistryAuthSecret(and the error it returns at lines 125–134) instructs them to createdevworkspace-backup-registry-auth. The two paths disagree on the expected name.Please align the names — either always look for/instruct on
constants.DevWorkspaceBackupAuthSecretName, or always honor the operator-configuredAuthSecretname in both lookup and the user-facing error. This was also raised in PR review feedback by dkwon17.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 63 - 98, The code currently looks up the workspace secret using secretName but EnsureRegistryAuthSecret (and its error/log messages) uses constants.DevWorkspaceBackupAuthSecretName, causing a mismatch; fix by making the operator-configured AuthSecret name authoritative: update EnsureRegistryAuthSecret (and any user-facing error/log text) to use the passed-in secretName instead of constants.DevWorkspaceBackupAuthSecretName so the lookupName logic, the Get calls, and the EnsureRegistryAuthSecret write/error messaging all reference the same secretName; ensure references to lookupName, secretName, and EnsureRegistryAuthSecret are consistent and adjust tests/logs accordingly.
♻️ Duplicate comments (1)
deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml (1)
221-224:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope the watch-secret label requirement to operator-managed copy-source secrets only.
The label sentence is still ambiguous and reads as if it applies to all
authSecretvalues. Please explicitly distinguish operator-managed/copy-source secrets vs user-provided/manual workspace secrets.✏️ Suggested doc tweak
- If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, - the operator will copy the secret from the operator namespace to the workspace namespace. - The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be - recognized by the operator. + If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, + the operator will copy the secret from the operator namespace to the workspace namespace. + For operator-managed/copy-source secrets, the source secret in the operator namespace must + contain "controller.devfile.io/watch-secret=true" so it can be recognized by the operator. + User-provided/manual secrets in workspace namespaces do not require this label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml` around lines 221 - 224, Clarify that the "controller.devfile.io/watch-secret=true" label is only required for operator-managed copy-source secrets when copyOperatorAuthSecret is true: update the description around the authSecret/copyOperatorAuthSecret text to state that if copyOperatorAuthSecret is true the operator will copy the secret from the operator namespace and that only those copy-source secrets must carry the controller.devfile.io/watch-secret=true label; explicitly note that user-provided/manual workspace secrets (i.e., when users set authSecret directly in the workspace namespace) do not need this label. Reference copyOperatorAuthSecret and authSecret and the watch-secret label in the updated sentence so it's unambiguous.
🧹 Nitpick comments (2)
pkg/secrets/backup.go (2)
87-87: 💤 Low valueLog key/value mismatch on the restore path.
lookupNamewas used for the workspace-namespaceGet(line 73), but this log line reportssecretNameas the secret it failed to find in the workspace namespace. On the restore path these can differ (line 68 overrideslookupNameto the predefined constant), so the log will misattribute the missing key. UselookupNamehere.♻️ Proposed fix
- log.Info("Registry auth secret not found in workspace namespace, checking operator namespace", "secretName", secretName) + log.Info("Registry auth secret not found in workspace namespace, checking operator namespace", + "workspaceSecretName", lookupName, "operatorSecretName", secretName)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` at line 87, The log message in pkg/secrets/backup.go incorrectly reports "secretName" when the code uses lookupName for the workspace-namespace Get; update the log.Info call that currently logs "secretName" to instead log the lookupName variable so the message reflects the actual attempted key (refer to the lookupName variable and the log.Info call that says "Registry auth secret not found in workspace namespace, checking operator namespace").
27-30: 💤 Low valueSwitch from deprecated
k8s.io/utils/pointertok8s.io/utils/ptr.The
pointerpackage is deprecated in favor ofptr. Useptr.Deref(replacingpointer.BoolDeref) to align with Kubernetes project migration guidance.♻️ Proposed refactor
- "k8s.io/utils/pointer" + "k8s.io/utils/ptr"- copyOperatorAuthSecret := pointer.BoolDeref( - dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, - false, - ) + copyOperatorAuthSecret := ptr.Deref( + dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, + false, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 27 - 30, The code imports the deprecated package alias pointer and uses pointer.BoolDeref; replace the import "k8s.io/utils/pointer" with "k8s.io/utils/ptr" (alias it as ptr if you used pointer) and update usages of pointer.BoolDeref to ptr.Deref (e.g., pointer.BoolDeref(someBoolPtr, false) -> ptr.Deref(someBoolPtr, false)); also change any other pointer.* helpers to their ptr.* equivalents (e.g., pointer.Bool -> ptr.Bool) so the file uses the non-deprecated ptr package and ptr.Deref consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/secrets/backup.go`:
- Around line 125-134: The user-facing error string in the fmt.Errorf return
(the block referencing constants.DevWorkspaceBackupAuthSecretName and
workspace.Namespace in the copyOperatorAuthSecret branch) concatenates sentences
without a space resulting in "...will not be copied.Please create..."; fix it by
inserting a space between the period and "Please" (or restructure the string
concatenation so sentences are separated with a space) so the message reads
"...will not be copied. Please create...".
---
Outside diff comments:
In `@pkg/secrets/backup.go`:
- Around line 63-98: The code currently looks up the workspace secret using
secretName but EnsureRegistryAuthSecret (and its error/log messages) uses
constants.DevWorkspaceBackupAuthSecretName, causing a mismatch; fix by making
the operator-configured AuthSecret name authoritative: update
EnsureRegistryAuthSecret (and any user-facing error/log text) to use the
passed-in secretName instead of constants.DevWorkspaceBackupAuthSecretName so
the lookupName logic, the Get calls, and the EnsureRegistryAuthSecret
write/error messaging all reference the same secretName; ensure references to
lookupName, secretName, and EnsureRegistryAuthSecret are consistent and adjust
tests/logs accordingly.
---
Duplicate comments:
In
`@deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`:
- Around line 221-224: Clarify that the
"controller.devfile.io/watch-secret=true" label is only required for
operator-managed copy-source secrets when copyOperatorAuthSecret is true: update
the description around the authSecret/copyOperatorAuthSecret text to state that
if copyOperatorAuthSecret is true the operator will copy the secret from the
operator namespace and that only those copy-source secrets must carry the
controller.devfile.io/watch-secret=true label; explicitly note that
user-provided/manual workspace secrets (i.e., when users set authSecret directly
in the workspace namespace) do not need this label. Reference
copyOperatorAuthSecret and authSecret and the watch-secret label in the updated
sentence so it's unambiguous.
---
Nitpick comments:
In `@pkg/secrets/backup.go`:
- Line 87: The log message in pkg/secrets/backup.go incorrectly reports
"secretName" when the code uses lookupName for the workspace-namespace Get;
update the log.Info call that currently logs "secretName" to instead log the
lookupName variable so the message reflects the actual attempted key (refer to
the lookupName variable and the log.Info call that says "Registry auth secret
not found in workspace namespace, checking operator namespace").
- Around line 27-30: The code imports the deprecated package alias pointer and
uses pointer.BoolDeref; replace the import "k8s.io/utils/pointer" with
"k8s.io/utils/ptr" (alias it as ptr if you used pointer) and update usages of
pointer.BoolDeref to ptr.Deref (e.g., pointer.BoolDeref(someBoolPtr, false) ->
ptr.Deref(someBoolPtr, false)); also change any other pointer.* helpers to their
ptr.* equivalents (e.g., pointer.Bool -> ptr.Bool) so the file uses the
non-deprecated ptr package and ptr.Deref consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a377f982-ce2d-4827-926d-c2adb24e358e
📒 Files selected for processing (12)
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.goapis/controller/v1alpha1/zz_generated.deepcopy.gocontrollers/backupcronjob/backupcronjob_controller_test.godeploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yamldeploy/deployment/kubernetes/combined.yamldeploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/deployment/openshift/combined.yamldeploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yamlpkg/config/sync.gopkg/secrets/backup.gopkg/secrets/backup_test.go
✅ Files skipped from review due to trivial changes (1)
- deploy/deployment/kubernetes/combined.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/config/sync.go
- apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go
- deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml
- deploy/deployment/openshift/combined.yaml
- deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml
- pkg/secrets/backup_test.go
- deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml
- apis/controller/v1alpha1/zz_generated.deepcopy.go
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
18bf441 to
385a1b3
Compare
|
@rohanKanojia: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
385a1b3 to
8afc9dc
Compare
rohanKanojia
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/secrets/backup.go (2)
34-40: ⚡ Quick winStale doc comment references the old function name.
The godoc on line 34 still says
GetRegistryAuthSecretwhile the function is nowGetNamespaceRegistryAuthSecret. Please align the comment with the new name (and ideally describe what "Namespace" means here since this thin wrapper just callsHandleRegistryAuthSecretwith an emptyoperatorConfigNamespace).📝 Suggested doc fix
-// GetRegistryAuthSecret retrieves the registry authentication secret for accessing backup images -// based on the operator configuration. +// GetNamespaceRegistryAuthSecret retrieves the registry authentication secret from the workspace +// namespace only (no fallback to the operator namespace). It is intended for the restore path where +// only user-provided, workspace-scoped credentials should be used. func GetNamespaceRegistryAuthSecret(ctx context.Context, c client.Client, workspace *dw.DevWorkspace,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 34 - 40, Update the stale godoc for GetNamespaceRegistryAuthSecret to reference its current name (replace "GetRegistryAuthSecret" with "GetNamespaceRegistryAuthSecret") and clarify that this wrapper calls HandleRegistryAuthSecret with an empty operatorConfigNamespace to use the default/operator-scoped config; mention what "Namespace" refers to (i.e., it operates for the workspace namespace by passing an empty operatorConfigNamespace). Ensure the comment sits immediately above the GetNamespaceRegistryAuthSecret function signature.
94-152: ⚡ Quick win
CopySecretis now narrowly purpose-built for the backup-auth secret; consider renaming/narrowing the signature.The function still has the generic name
CopySecret(... sourceSecret *corev1.Secret ...), but it ignoressourceSecret.Name/sourceSecret.Namespaceand hardcodes the destination name toconstants.DevWorkspaceBackupAuthSecretName. A future caller passingsourceSecretnamed "foo" would silently end up with a workspace secret nameddevworkspace-backup-registry-auth, which is surprising.Two reasonable options:
- Rename to something like
EnsureBackupAuthSecretInWorkspaceand shrink the signature to just the bits actually consumed (data map[string][]byte,secretType corev1.SecretType), making it explicit that this is not a general-purpose copy.- Keep the generic name but actually honor
sourceSecret.Name(and require it to equal the constant when called fromHandleRegistryAuthSecret), so the function stays reusable.Also note: this nicely pairs with the previous comment — if
Registry.AuthSecretbecomes the operator-namespace source name, option (1) becomes the cleaner separation, since the source/destination naming differ.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 94 - 152, The CopySecret function is actually specialized for the backup-auth secret but still takes a generic sourceSecret and ignores its name; rename CopySecret to EnsureBackupAuthSecretInWorkspace (or similar) and shrink its signature to only accept the fields it uses (e.g., data map[string][]byte, secretType corev1.SecretType, workspace *dw.DevWorkspace, scheme *runtime.Scheme, ctx/client/log) so callers can't accidentally expect a generic copy; update all callers (e.g., HandleRegistryAuthSecret) to pass sourceSecret.Data and sourceSecret.Type (or keep a thin adapter if needed), keep the hardcoded destination name constants.DevWorkspaceBackupAuthSecretName and the existing controllerutil.SetControllerReference, and preserve the existing Get/Create/IsAlreadyExists race handling and logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/secrets/backup.go`:
- Around line 67-89: The code currently ignores the admin-configured name in
dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret and always looks up
constants.DevWorkspaceBackupAuthSecretName in the operator namespace; change the
operator-namespace lookup (the c.Get call that populates registryAuthSecret) to
use the configured name
(dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret) instead of the
constant, and update the preceding error/log message to reflect that the
configured name will be used for the operator-namespace lookup; also update
related tests (backup_test.go) to seed the operator namespace with the
configured secret name rather than constants.DevWorkspaceBackupAuthSecretName.
---
Nitpick comments:
In `@pkg/secrets/backup.go`:
- Around line 34-40: Update the stale godoc for GetNamespaceRegistryAuthSecret
to reference its current name (replace "GetRegistryAuthSecret" with
"GetNamespaceRegistryAuthSecret") and clarify that this wrapper calls
HandleRegistryAuthSecret with an empty operatorConfigNamespace to use the
default/operator-scoped config; mention what "Namespace" refers to (i.e., it
operates for the workspace namespace by passing an empty
operatorConfigNamespace). Ensure the comment sits immediately above the
GetNamespaceRegistryAuthSecret function signature.
- Around line 94-152: The CopySecret function is actually specialized for the
backup-auth secret but still takes a generic sourceSecret and ignores its name;
rename CopySecret to EnsureBackupAuthSecretInWorkspace (or similar) and shrink
its signature to only accept the fields it uses (e.g., data map[string][]byte,
secretType corev1.SecretType, workspace *dw.DevWorkspace, scheme
*runtime.Scheme, ctx/client/log) so callers can't accidentally expect a generic
copy; update all callers (e.g., HandleRegistryAuthSecret) to pass
sourceSecret.Data and sourceSecret.Type (or keep a thin adapter if needed), keep
the hardcoded destination name constants.DevWorkspaceBackupAuthSecretName and
the existing controllerutil.SetControllerReference, and preserve the existing
Get/Create/IsAlreadyExists race handling and logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48f686d6-3831-4b20-8a0e-357bc50c82c8
📒 Files selected for processing (2)
pkg/secrets/backup.gopkg/secrets/backup_test.go
|
Hi! I'm che-ai-assistant — I help with your pull requests. I check for new commands every 5m0s (if I am not busy :) ). Available commands:
|
…ator namespace to workspace namespace for backup/restore Only copy the registry auth secret from the operator namespace to the workspace namespace when it is configured inside DevWorkspaceOperatorConfig. Always give precedence to the user-defined secret named devworkspace-backup-registry-auth in the workspace namespace; do not modify it when it's present while copying. Co-Authored-By: Claude Sonnet 4.5 <[email protected]> Signed-off-by: Rohan Kumar <[email protected]>
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
8afc9dc to
3cec08a
Compare
|
For scenario 1, if no auth secret is created, the backup cron job pod should still start (this is the use case when using the internal openshift registry, or, a registry where no auth is required) Right now I am getting this error: |
|
@dkwon17 : Do you mean to say in this case we should just log a message instead of throwing an error? |
…reated Signed-off-by: Rohan Kumar <[email protected]>
The BackupCronJob controller initialized its root logger with V(1), making all controller logs hidden unless the operator was started with increased zap verbosity. Remove the verbosity override from the controller logger to align with other controllers and use default log visibility consistently. Signed-off-by: Rohan Kumar <[email protected]>
|
@dkwon17 : I've modified code to just log a message instead of throwing error. Now I can confirm that backup pod gets created for openshift internal registry. I've added new test scenarios for openshift interal registry and external registry with no auth. |
There was a problem hiding this comment.
🧹 Nitpick comments (6)
pkg/secrets/backup.go (3)
98-156: 💤 Low valueRefactor looks good; consider documenting the naming model.
The Get-then-Create flow with
IsAlreadyExistsrace handling correctly enforces never-overwrite semantics, and returning the pre-existing secret without modification matches the contract in the doc comment. Owner reference is set on the freshly created copy only, so user-provided secrets are not implicitly garbage-collected on workspace deletion — which is the safer default.One follow-up worth surfacing in user-facing docs (CRD comment for
RegistryConfig.AuthSecretand/or the godoc onHandleRegistryAuthSecret): the workspace-namespace secret name is always the canonicaldevworkspace-backup-registry-auth, whileAuthSecretonly configures the source name in the operator namespace. This mismatch came up in the PR discussion and is easy to miss from just reading the field name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 98 - 156, The docs should explicitly state that the workspace-namespace secret is always named by the operator constant (constants.DevWorkspaceBackupAuthSecretName / "devworkspace-backup-registry-auth") and that RegistryConfig.AuthSecret refers only to the source secret name in the operator namespace; update the CRD field comment for RegistryConfig.AuthSecret and the godoc for HandleRegistryAuthSecret (and/or pkg/secrets.CopySecret) to mention this naming model and the non-overwrite behavior so users are not surprised by the canonical workspace secret name.
34-40: 💤 Low valueStale godoc references the prior function name.
The doc comment starts with
GetRegistryAuthSecret retrieves ...but the function is nowGetNamespaceRegistryAuthSecret. Godoc/golint tooling treats the first word as the documented identifier, so this generates misleading documentation.✏️ Proposed fix
-// GetRegistryAuthSecret retrieves the registry authentication secret for accessing backup images -// based on the operator configuration. +// GetNamespaceRegistryAuthSecret retrieves the registry authentication secret for accessing +// backup images from the workspace namespace only (no operator-namespace fallback). func GetNamespaceRegistryAuthSecret(ctx context.Context, c client.Client, workspace *dw.DevWorkspace,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 34 - 40, Update the stale godoc so its opening identifier matches the actual function name: change the comment that currently begins "GetRegistryAuthSecret retrieves ..." to begin "GetNamespaceRegistryAuthSecret retrieves ..." (and verify the rest of the sentence still accurately describes behavior); locate the comment above the GetNamespaceRegistryAuthSecret function in pkg/secrets/backup.go and update it accordingly (no other code changes to GetNamespaceRegistryAuthSecret or the call to HandleRegistryAuthSecret are required).
79-81: 💤 Low valueLog key labels could be clearer about which name lives in which namespace.
The message reads "not found in workspace namespace, checking operator namespace", but the
secretNamekey carries the operator-namespace name (authSecretName). An operator reading this log will likely assumesecretNamerefers to the missing workspace-NS secret. Splitting into two keys removes the ambiguity, especially since the workspace NS always looks up the canonical name regardless ofAuthSecret.✏️ Suggested clarification
- log.Info("Registry auth secret not found in workspace namespace, checking operator namespace", - "secretName", authSecretName, - "operatorNamespace", operatorConfigNamespace) + log.Info("Registry auth secret not found in workspace namespace, falling back to operator namespace", + "workspaceSecretName", constants.DevWorkspaceBackupAuthSecretName, + "workspaceNamespace", workspace.Namespace, + "operatorSecretName", authSecretName, + "operatorNamespace", operatorConfigNamespace)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 79 - 81, The log is ambiguous about which secret name lives in which namespace; update the log.Info call to emit separate, explicit keys so readers can tell the operator-namespace secret name vs. the workspace namespace context (e.g. add "operatorSecretName" with authSecretName and "workspaceNamespace" with the workspace namespace variable used by the lookup). Locate the log.Info invocation (log.Info) and replace the single "secretName" key with distinct keys referencing authSecretName for the operator secret and the existing workspace namespace/secret variable used by the workspace lookup.pkg/secrets/backup_test.go (2)
191-215: 💤 Low valueOptional: also assert the copied secret carries the expected label and owner reference.
CopySecretis documented (and implemented) to set bothDevWorkspaceWatchSecretLabeland a controller reference to theDevWorkspace. Adding those assertions guards against future regressions where the copy succeeds but loses the metadata needed for cache filtering / GC on workspace deletion.✏️ Suggested additions
By("verifying the secret was created in the workspace namespace") copiedSecret := &corev1.Secret{} err = fakeClient.Get(ctx, client.ObjectKey{ Name: constants.DevWorkspaceBackupAuthSecretName, Namespace: workspaceNS, }, copiedSecret) Expect(err).NotTo(HaveOccurred()) Expect(copiedSecret.Data["auth"]).To(Equal([]byte("operator-credentials"))) + Expect(copiedSecret.Labels).To(HaveKeyWithValue(constants.DevWorkspaceWatchSecretLabel, "true")) + Expect(copiedSecret.OwnerReferences).To(HaveLen(1)) + Expect(copiedSecret.OwnerReferences[0].Name).To(Equal(workspace.Name)) + Expect(copiedSecret.OwnerReferences[0].Kind).To(Equal("DevWorkspace"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup_test.go` around lines 191 - 215, Update the test that verifies secret copying (the It block calling secrets.HandleRegistryAuthSecret) to also assert that the copied secret includes the expected DevWorkspaceWatchSecretLabel and has a controller ownerReference pointing to the DevWorkspace; locate the assertions after retrieving copiedSecret and add checks for copiedSecret.Labels[constants.DevWorkspaceWatchSecretLabel] (or the exact label key) and that copiedSecret.OwnerReferences contains an entry with Controller=true and a UID/Name matching the workspace created by makeWorkspace (this ensures CopySecret set the label and ownerRef correctly).
246-256: 💤 Low valueOptional: assert that no workspace secret was created on the error path.
Currently the test only asserts the returned error is
IsNotFound. Adding aGetagainst the workspace namespace would pin down that the operator does not leave a partially-initialized/emptydevworkspace-backup-registry-authbehind when the configured operator secret is missing.✏️ Suggested addition
result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log) Expect(err).To(HaveOccurred()) Expect(result).To(BeNil()) Expect(k8sErrors.IsNotFound(err)).To(BeTrue(), "Should return a NotFound error when secret doesn't exist in operator namespace") + + By("verifying no secret was created in the workspace namespace") + leftover := &corev1.Secret{} + getErr := fakeClient.Get(ctx, client.ObjectKey{ + Name: constants.DevWorkspaceBackupAuthSecretName, + Namespace: workspaceNS, + }, leftover) + Expect(k8sErrors.IsNotFound(getErr)).To(BeTrue())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup_test.go` around lines 246 - 256, The test for HandleRegistryAuthSecret should also assert that no workspace secret was created on the error path: after calling secrets.HandleRegistryAuthSecret with the fake client and expecting a NotFound error, attempt to Get the Secret named constants.DevWorkspaceBackupAuthSecretName (or the hardcoded "devworkspace-backup-registry-auth" used by makeConfig/makeWorkspace) from the workspace namespace (workspaceNS) via the fakeClient and assert that the Get returns a NotFound error (i.e., no secret exists); this verifies the operator did not create a partially-initialized workspace secret when the operator namespace secret is missing.controllers/backupcronjob/backupcronjob_controller_test.go (1)
538-574: 💤 Low valueOptional: tighten the spec name to reflect what's being validated.
This case primarily verifies that a pre-existing workspace-scoped secret (canonical name) takes precedence and is honored without needing the operator-NS source. A name like
"creates a Job for a DevWorkspace stopped with no previous backup when a workspace-scoped auth secret already exists"reads more aligned with the assertion, sinceAuthSecret: "my-secret"is intentionally never resolved in this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/backupcronjob/backupcronjob_controller_test.go` around lines 538 - 574, Update the test description string for the It(...) case that currently reads "creates a Job for a DevWorkspace stopped with no previous backup and local auth registry" to a tighter, more accurate name such as "creates a Job for a DevWorkspace stopped with no previous backup when a workspace-scoped auth secret already exists" so the spec reflects that the workspace-scoped secret (the canonical name created via createAuthSecret) is honored and the OperatorConfig.AuthSecret ("my-secret") is not used in this path; locate the It(...) block in the backupcronjob_controller_test (the test where dwoc, dw, pvc and authSecret are created and reconciler.executeBackupSync is asserted) and change only the string literal describing the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@controllers/backupcronjob/backupcronjob_controller_test.go`:
- Around line 538-574: Update the test description string for the It(...) case
that currently reads "creates a Job for a DevWorkspace stopped with no previous
backup and local auth registry" to a tighter, more accurate name such as
"creates a Job for a DevWorkspace stopped with no previous backup when a
workspace-scoped auth secret already exists" so the spec reflects that the
workspace-scoped secret (the canonical name created via createAuthSecret) is
honored and the OperatorConfig.AuthSecret ("my-secret") is not used in this
path; locate the It(...) block in the backupcronjob_controller_test (the test
where dwoc, dw, pvc and authSecret are created and reconciler.executeBackupSync
is asserted) and change only the string literal describing the test.
In `@pkg/secrets/backup_test.go`:
- Around line 191-215: Update the test that verifies secret copying (the It
block calling secrets.HandleRegistryAuthSecret) to also assert that the copied
secret includes the expected DevWorkspaceWatchSecretLabel and has a controller
ownerReference pointing to the DevWorkspace; locate the assertions after
retrieving copiedSecret and add checks for
copiedSecret.Labels[constants.DevWorkspaceWatchSecretLabel] (or the exact label
key) and that copiedSecret.OwnerReferences contains an entry with
Controller=true and a UID/Name matching the workspace created by makeWorkspace
(this ensures CopySecret set the label and ownerRef correctly).
- Around line 246-256: The test for HandleRegistryAuthSecret should also assert
that no workspace secret was created on the error path: after calling
secrets.HandleRegistryAuthSecret with the fake client and expecting a NotFound
error, attempt to Get the Secret named
constants.DevWorkspaceBackupAuthSecretName (or the hardcoded
"devworkspace-backup-registry-auth" used by makeConfig/makeWorkspace) from the
workspace namespace (workspaceNS) via the fakeClient and assert that the Get
returns a NotFound error (i.e., no secret exists); this verifies the operator
did not create a partially-initialized workspace secret when the operator
namespace secret is missing.
In `@pkg/secrets/backup.go`:
- Around line 98-156: The docs should explicitly state that the
workspace-namespace secret is always named by the operator constant
(constants.DevWorkspaceBackupAuthSecretName /
"devworkspace-backup-registry-auth") and that RegistryConfig.AuthSecret refers
only to the source secret name in the operator namespace; update the CRD field
comment for RegistryConfig.AuthSecret and the godoc for HandleRegistryAuthSecret
(and/or pkg/secrets.CopySecret) to mention this naming model and the
non-overwrite behavior so users are not surprised by the canonical workspace
secret name.
- Around line 34-40: Update the stale godoc so its opening identifier matches
the actual function name: change the comment that currently begins
"GetRegistryAuthSecret retrieves ..." to begin "GetNamespaceRegistryAuthSecret
retrieves ..." (and verify the rest of the sentence still accurately describes
behavior); locate the comment above the GetNamespaceRegistryAuthSecret function
in pkg/secrets/backup.go and update it accordingly (no other code changes to
GetNamespaceRegistryAuthSecret or the call to HandleRegistryAuthSecret are
required).
- Around line 79-81: The log is ambiguous about which secret name lives in which
namespace; update the log.Info call to emit separate, explicit keys so readers
can tell the operator-namespace secret name vs. the workspace namespace context
(e.g. add "operatorSecretName" with authSecretName and "workspaceNamespace" with
the workspace namespace variable used by the lookup). Locate the log.Info
invocation (log.Info) and replace the single "secretName" key with distinct keys
referencing authSecretName for the operator secret and the existing workspace
namespace/secret variable used by the workspace lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26906af0-afec-45be-abdd-2eb1d2d08e45
📒 Files selected for processing (4)
controllers/backupcronjob/backupcronjob_controller_test.gomain.gopkg/secrets/backup.gopkg/secrets/backup_test.go
✅ Files skipped from review due to trivial changes (1)
- main.go
3 participants
What does this PR do?
Only copy the registry auth secret from the operator namespace to the workspace namespace when it is configured inside DevWorkspaceOperatorConfig. Always give precedence to the user-defined secret named devworkspace-backup-registry-auth in the workspace namespace; do not modify it when it's present while copying.
What issues does this PR fix or reference?
https://redhat.atlassian.net/browse/CRW-10758
Is it tested? How?
Scenario 1: AuthSecret Not Configured (Secure by Default)
Expected: Operator will NOT copy secrets. User must provide their own.
kubectl patch devworkspace test-workspace-no-copy -n test-workspace-no-auth \ --type merge -p '{"spec":{"started":false}}'Expected log:
kubectl get secret devworkspace-backup-registry-auth -n test-workspace-no-auth # Expected: Error from server (NotFound)Scenario 2: AuthSecret Configured - Operator Copies Secret
Expected: Operator copies secret from operator namespace to workspace namespace.
kubectl patch devworkspace test-workspace-with-copy -n test-workspace-copy \ --type merge -p '{"spec":{"started":false}}'Scenario 3: User-Provided Secret NEVER gets Overwritten
Expected: User's secret is respected and NEVER overwritten, even if operator has different credentials.
kubectl patch devworkspace test-workspace-user-secret -n test-workspace-user-secret \ --type merge -p '{"spec":{"started":false}}'Scenario 4: AuthSecret Configured but Operator Secret Missing
Expected: Operator logs error that it cannot find the configured secret in operator namespace.
kubectl patch devworkspace test-workspace-missing -n test-workspace-missing-secret \ --type merge -p '{"spec":{"started":false}}'Expected log:
kubectl get secret devworkspace-backup-registry-auth -n test-workspace-missing-secret # Expected: Error from server (NotFound)Scenario 5: OpenShift Internal Registry - No Auth Required
Expected: Backup works without authentication when using OpenShift internal registry service URL.
kubectl patch devworkspace test-workspace-internal -n test-workspace-internal-registry \ --type merge -p '{"spec":{"started":false}}'Expected log:
# The backup image should exist in the OpenShift internal registry oc get imagestream -n backups test-workspace-internalScenario 6: Public/Anonymous Registry - No Auth Required
Expected: Backup works without authentication for registries that allow anonymous push (rare but valid use case).
kubectl patch devworkspace test-workspace-public -n test-workspace-public-registry \ --type merge -p '{"spec":{"started":false}}'kubectl get jobs -n test-workspace-public-registry -l controller.devfile.io/devworkspace_name=test-workspace-publicPR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
Bug Fixes
Tests
Style